AIX/XCOFF support for fibers #7338
Conversation
The fibers PR neglected to import the AIX-specific code from Boost's contexts library. This PR reinstates them and adds them to autoconf. May be required for non-_CALL_ELF==2 Linux, since that shares the AIX ABI (like, pretty much every non-LE enterprise distro, i.e. EL7).
|
Marked as draft because I haven't tested; the PHP runtime works but I'm not familiar with fibers just yet. If it works, just unmark it as such. |
|
I guess this is why I marked it draft: Poking at it, but I didn't spy any site-local modifications to PHP's imported files. I could also just fall back to no asm as well, like #7226 did. |
|
To be sure I'm understanding the above correctly, it's failing on the store instruction for the context function? |
|
Seems so. I compared the ELF version and it looks fairly similar, though the use of stw for presumably 32-bit seems sus compared to the std in ELF. ucontext_t works for now, so this PR isn't too high priority. |
|
It appears to be using |
It is 64-bit, but it's selecting the 64-bit assembly properly it seems, per comparing the two files with the compiled file: 32-bit: #.make_fcontext:
# save return address into R6
mflr 6
# first arg of make_fcontext() == top address of context-function
# shift address in R3 to lower 16 byte boundary
clrrwi 3, 3, 4
# reserve space for context-data on context-stack
# including 64 byte of linkage + parameter area (R1 % 16 == 0)
subi 3, 3, 336
# third arg of make_fcontext() == address of context-function
stw 5, 240(3)64-bit: #._make_fcontext:
# save return address into R6
mflr 6
# first arg of make_fcontext() == top address of context-function
# shift address in R3 to lower 16 byte boundary
clrrwi 3, 3, 4
# reserve space for context-data on context-stack
# including 64 byte of linkage + parameter area (R1 % 16 == 0)
subi 3, 3, 248
# third arg of make_fcontext() == address of context-function
stw 5, 176(3)And GDB: |
|
Oh, I missed that |
|
Doesn't seem to change it; PC is at the same place on crash site. I do note r3 (first arg reg) does seem a bit funny: If I try to print out the memory there: Hmmm. Looking at the code there, ZEND_API bool zend_fiber_init_context(zend_fiber_context *context, void *kind, zend_fiber_coroutine coroutine, size_t stack_size)
{
context->stack = zend_fiber_stack_allocate(stack_size);
if (UNEXPECTED(!context->stack)) {
return false;
}
#ifdef ZEND_FIBER_UCONTEXT
// [...]
#else
// Stack grows down, calculate the top of the stack. make_fcontext then shifts pointer to lower 16-byte boundary.
void *stack = (void *) ((uintptr_t) context->stack->pointer + context->stack->size);
context->handle = make_fcontext(stack, context->stack->size, zend_fiber_trampoline);
ZEND_ASSERT(context->handle != NULL && "make_fcontext() never returns NULL");
#endifSomething in |
Not terribly surprised – I assume the boost authors know what they're doing.
Try replacing the stack allocation to use PHP's memory allocator: replace the |
|
Build with The returned pointer from mmap looks fine to me, but it gets mangled somewhere once it hits |
|
gdb doesn't want to put a breakpoint on the symbol (if I do, it just goes to the crashsite instead), but putting a BP onto the entry point's address works. I think the code from boost mangles the r3 value 🤨 |
I think it's the |
|
Well... # first arg of make_fcontext() == top address of context-function
# shift address in R3 to lower 16 byte boundary
clrrwi 3, 3, 4Ugh, should that be clrrdi instead? Looks like it just masks out the bottom bits, but it's clobbering the top bits with it. How could this have ever worked? (I even ran the test suite for boost::context.... or at least tried, anyways. All but one test passed, and it was unrelated to this....) |
Intuitively I would expect all the word instructions to need to be double-word on 64-bit. I guess try changing both stw and clrrwi to std and clrrdi. |
|
Using doubleword instructions, that seems to fix the address error, but the code seems unaware of function descriptors (in AIX/ELFv1 ABI, it's IIRC a pc/r2/"env" tuple); it's jumping right to one. The code in Boost explicitly states it's for AIX, so I'm not sure why it's so blatantly wrong. |
|
(Ironically, the code in the ppc64 ELFv1 case might be able to handle it because again, 99% identical ABI, just needs adapting to IBM assembler...) |
|
How did the address to |
|
It's probably not - GDB probably considers it to be because it's the last known symbol in php's i.e. you can see that func isn't ~82k instructions long |
That makes sense, thanks for the explanation. I wonder then if |
|
I think stw would truncate it to 32-bits which might be bad; I think we're missing the TOC storage (r2) in make and loading in jump, comparing with the ELF _CALL_ELF==1 case. |
I would think so too, but the 64-bit PPC assembly for mac also uses
Seems like a reasonable thing to try I guess, curious if that makes a difference. |
|
Getting closer; we no longer barf in |
|
Seems like jump_fcontext needs fixing next: The SP seems clobbered. |
|
I filed an issue on boostorg/context#180 because the assembly not matching the ABI at all is confusing. There has to be a better explanation... |
|
Seems there's a lot more logic around copying the |
|
Yup, seems like making those adaptations will be needed. I'm also going to see what code GCC will emit in a smaller, isolated case from C, which I can reason with more. |
|
OK, I'm once again comparing the elf/xcoff versions and I'm not sure where you're seeing where jump_fcontext in the ELF version handles args being bumped for the struct pointer in r3. I do notice "zero in r3 indicates first jump to context-function" not in the XCOFF version, but I can't think that'd be it? Otherwise, they seem the same outside of r2. I might need to get access to a Linux box to compare this on... |
|
I was referring to the branch on I put the following together quickly based on comparing the ELF and XCOFF versions. Not sure if my syntax is correct on the label and branch, but it demonstrates the differences I was referring to with /*
Copyright Oliver Kowalke 2009.
Distributed under the Boost Software License, Version 1.0.
(See accompanying file LICENSE_1_0.txt or copy at
http://www.boost.org/LICENSE_1_0.txt)
*/
.align 2
.globl .jump_fcontext
.jump_fcontext:
# reserve space on stack
subi 1, 1, 184
std 2, 0(1) # save TOC !! ADDED
std 13, 0(1) # save R13
std 14, 8(1) # save R14
std 15, 16(1) # save R15
std 16, 24(1) # save R16
std 17, 32(1) # save R17
std 18, 40(1) # save R18
std 19, 48(1) # save R19
std 20, 56(1) # save R20
std 21, 64(1) # save R21
std 22, 72(1) # save R22
std 23, 80(1) # save R23
std 24, 88(1) # save R24
std 25, 96(1) # save R25
std 26, 104(1) # save R26
std 27, 112(1) # save R27
std 29, 120(1) # save R28
std 29, 128(1) # save R29
std 30, 136(1) # save R30
std 31, 144(1) # save R31
std 3, 152(1) # save hidden
# save CR
mfcr 0
std 0, 160(1)
# save LR
mflr 0
std 0, 168(1)
# save LR as PC
std 0, 176(1)
# store RSP (pointing to context-data) in R6
mr 6, 1
# restore RSP (pointing to context-data) from R4
mr 1, 4
ld 2, 0(1) # restore TOC !! ADDED
ld 13, 0(1) # restore R13
ld 14, 8(1) # restore R14
ld 15, 16(1) # restore R15
ld 16, 24(1) # restore R16
ld 17, 32(1) # restore R17
ld 18, 40(1) # restore R18
ld 19, 48(1) # restore R19
ld 20, 56(1) # restore R20
ld 21, 64(1) # restore R21
ld 22, 72(1) # restore R22
ld 23, 80(1) # restore R23
ld 24, 88(1) # restore R24
ld 25, 96(1) # restore R25
ld 26, 104(1) # restore R26
ld 27, 112(1) # restore R27
ld 28, 120(1) # restore R28
ld 29, 128(1) # restore R29
ld 30, 136(1) # restore R30
ld 31, 144(1) # restore R31
ld 3, 152(1) # restore hidden
# restore CR
ld 0, 160(1)
mtcr 0
# restore LR
ld 0, 168(1)
mtlr 0
# load PC
ld 0, 176(1)
# restore CTR
mtctr 0
# adjust stack
addi 1, 1, 184
cmpdi 3, 0 # !! ADDED
beq .use_entry_arg # !! ADDED
# return transfer_t
std 6, 0(3)
std 5, 8(3)
# jump to context
bctr
.use_entry_arg: # !! ADDED
# copy transfer_t into transfer_fn arg registers
mr 3, 6
mr 4, 5
# jump to context
bctrThe mac version has this similar logic, perhaps it would be helpful. |
|
Ah, that'd be the changes I was porting after comparing both. I thought you meant something further above in i.e. the load/store multiple registers clumps. |
|
There is the store on |
|
I didn't try changing the store/load TOC stuff yet but I think that needs to be instead of the r13, since they occupy the same space on the stack and the calling convention docs seem to indicate it's reserved. I'm building with adding the branch, and if that doesn't work, changing the register loaded there. |
|
Yeah, after your suggested changes (I'll push those), I don't see any change. If you're interested I can get you access to a box. Likewise, I'll try to get access to a Linux box, because this behaviour doesn't make sense if we're basically exactly matching it now and the ABIs are shared. |
D'oh, yeah, missed removing the store of r13, so that would squash that anyway. Assuming you fixed that before trying?
Sure, I'd be willing to mess around a bit. Not entirely sure when I'll get time for it though, but if you can leave the time period open-ended I'll give it try. |
|
Already did. I'll try to arrange an account for you; thankfully, there's a workaround (ucontext_t) at least, so it's not too urgent. |
|
I did notice an inconsistency between make_fcontext; I'm seeing if that changes anything... |
|
FINALLY (using the example from RFC, changed for current API) |
|
Awesome! Try running the fiber tests. |
|
First pass |
|
Looking, all of the failing tests seem to be crashes; oddly, the ones I've tried all seem to be in |
|
Forgot to include a backtrace; here's error-reporting.php; it's an |
|
I suspect that the pointer to |
Matches the Linux ELFv1 case, and might resolve an issue with restoring from a fiber.
|
I agree it's getting borked, but I suspect it's affecting more than the zval, but also impacting other things; I noticed |
|
Much better! I think it'll also be worth taking the changes to upstream Boost as well. |
|
Fantastic! So essentially it was modifying the XCOFF version to be inline with the ELF version when
Absolutely. It's nice that we'll be able to give back to that project. 👍 |
|
Yes, there were basically a few spots I missed that I neglected to catch, basically. |
The fibers PR neglected to import the AIX-specific code from Boost's contexts library. This PR reinstates them and adds them to autoconf.
May be required for non-_CALL_ELF==2 Linux, since that shares the AIX ABI (like, pretty much every non-LE enterprise distro, i.e. EL7).